Skip to content

Conversation

@mringwal
Copy link
Contributor

@mringwal mringwal commented Jan 6, 2025

The current driver reserves a larger buffer for the BTstack Pre-Buffer in memory, but does not use it when receiving an HCI packet from CYW43 driver. As some BTstack events will use this buffer, this causes an out-of-bound write / crash with BTstack v1.6.2 or higher.

Note: the 4-byte CYW43 header implicitly provided a 3-byte pre-buffer, which worked for BTstack v1.6.1 and earlier. In BTstack v1.6.2, the pre-buffer was increased which causes this bug to unhide.

This replaces #2157 (comment)

@peterharperuk peterharperuk changed the base branch from master to develop January 6, 2025 12:16
@peterharperuk
Copy link
Contributor

I changed the branch to develop

@lurch
Copy link
Contributor

lurch commented Jan 16, 2025

I've got no idea why this was assigned to me (I have no BTstack knowledge), so assigning to Peter instead.

@lurch lurch assigned peterharperuk and unassigned lurch Jan 16, 2025
@peterharperuk
Copy link
Contributor

Oops - I should get this in. Will just run a quick test.

@peterharperuk peterharperuk added this to the 2.1.1 milestone Jan 16, 2025
peterharperuk
peterharperuk previously approved these changes Jan 16, 2025
@peterharperuk
Copy link
Contributor

@kilograham Can we merge this?

@kilograham
Copy link
Contributor

Also i don't understand this at all - maybe a comment about what the various ranges of the buffer are used for (particularly the 4 vs the PREBUFFER - why are they not one and the same, and where alignment comes into play)

@mringwal
Copy link
Contributor Author

I have improved the documentation and merged the CYW43 packet header buffer with BTstack's pre-buffer, by asserting that HCI_INCOMING_PRE_BUFFER_SIZE is >= CYW43_PACKET_HEADER_SIZE (4).

It's better to read now. However, the build fails for me with "#error HCI_INCOMING_PRE_BUFFER_SIZE not defined or smaller than 4 (CYW43_PACKET_HEADER_SIZE) bytes. Please update btstack_config.h".

@peterharperuk Could you try to find out, where HCI_INCOMING_PRE_BUFFER_SIZE is set?
I get the feeling that different btstack_config.h are used to compile btstack_hci_transport_cyw43.c and the rest of BTstack. Both need to see the same value of HCI_INCOMING_PRE_BUFFER_SIZE to avoid memory issues. E.g. the GATT Client has some requirements on the pre-buffer size, which needs to be provided by btstack_hci_transport_cyw43.c. Feel free to ask for specific details.

@mringwal mringwal force-pushed the btstack-incoming-hci-buffer-fix branch from 431be34 to 6426636 Compare January 21, 2025 15:10
@peterharperuk
Copy link
Contributor

Could you try to find out, where HCI_INCOMING_PRE_BUFFER_SIZE is set?

As far as I can see, it's coming from btstack/src/hci.h

// BNEP may uncompress the IP Header by 16 bytes, GATT Client requires six additional bytes for long characteristic reads
// wih service_id + connection_id
#ifndef HCI_INCOMING_PRE_BUFFER_SIZE
#ifdef ENABLE_CLASSIC
#define HCI_INCOMING_PRE_BUFFER_SIZE (16 - HCI_ACL_HEADER_SIZE - 4)
#else
#define HCI_INCOMING_PRE_BUFFER_SIZE 6
#endif
#endif

@mringwal
Copy link
Contributor Author

@peterharperuk Does it compile for you? Also: where is support for Classic and/or LE defined? If Classic would be defined, then HCI_INCOMING_PRE_BUFFER_SIZE should be (16 - HCI_ACL_HEADER_SIZE - 4) = 8, which is larger than 4. I wasn't able to compile this as it seems to have picked up the value 2 from the #else branch - without Classic.

@peterharperuk
Copy link
Contributor

If you link to pico_btstack_classic it defines ENABLE_CLASSIC and if you link to pico_btstack_ble it defines ENABLE_BLE. Most of the examples are linking to both. https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_btstack/CMakeLists.txt#L107

Let me do some more testing. We haven't updated btstack to v1.6.2, we're still using v1.6.1. Should we update?

@mringwal
Copy link
Contributor Author

Interesting.

So, how does btstack_hci_transport_cyw43.c know which HCI_INCOMING_PRE_BUFFER_SIZE to use? Is it part of pico_btstack_classic as well as pico_btstack_ble.

If in doubt, we can just use a rather large HCI_INCOMING_PRE_BUFFER_SIZE, e.g. 16, for . btstack_hci_transport_cyw43. We don't plan on increasing the pre-buffer and just did a larger rewrite of the GATT Client logic, which is a big user of HCI_INCOMING_PRE_BUFFER_SIZE.

Yes, you should update to master (1.6.2 + one additional fix).

@peterharperuk
Copy link
Contributor

There's only on version of btstack_hci_transport_cyw43.c. So if you link to classic you get 8. If you ONLY link to ble you get 2. If you link to both you get 8? Is that wrong? I'm fine with us adding a set number to be safe. I thought your change was ok though?

@mringwal
Copy link
Contributor Author

btstack_hci_transport_cyw43.c should use whatever hci.h choses based on ENABLE_CLASSIC or not.
If that holds true, everything is fine.

(I don't understand the pico-sdk build system. I generally assume that a particular .c file is only compiled once into an .o file (by this having a fixed value for HCI_INCOMING_PRE_BUFFER_SIZE) and then I don't understand how an example can link to pico_btstack_classic as well as pico_btstack_ble at the same time).

@peterharperuk
Copy link
Contributor

Everything is a cmake interface library in the sdk https://cmake.org/cmake/help/latest/command/add_library.html#interface-libraries

@mringwal mringwal force-pushed the btstack-incoming-hci-buffer-fix branch from 6426636 to c88df1c Compare January 22, 2025 07:30
@mringwal
Copy link
Contributor Author

@peterharperuk Thanks. Interesting, I need to revisit my concept of "library". Didn't know it could set defines for the main project (which totally makes sense if you start to thing about it).

Anyway. There's a new force-pushed version. It validates the size of the outgoing buffer and throws an error if needed, as the outgoing buffer is owned by hci.c. For the incoming buffer, which we need to provide, it increases it if needed.

By this, pico-sdk should compile with the current (older) BTstack version as well as newer ones (v1.6.2 will require a larger pre-buffer for LE examples).

@peterharperuk
Copy link
Contributor

Tested with 1.6.1 and 1.6.2 #2202

@kilograham kilograham merged commit 25069f5 into raspberrypi:develop Jan 29, 2025
4 checks passed
will-v-pi pushed a commit to will-v-pi/pico-sdk that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants